Skip to content

Switch Kálmán filters to use Joseph form #1048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Switch Kálmán filters to use Joseph form #1048

wants to merge 1 commit into from
+23 −5

Conversation

@stephenswat
Copy link
Member

@stephenswat stephenswat commented Jul 1, 2025

This commit switches the Kálmán filters to use the Joseph form covariance update which is known to be usable for any gain matrix $K$ even if the gain matrix is distorted by numerical imprecision. Empirically, this seems to resolve quite a few issues with our filtering algorithms.

@stephenswat stephenswat added improvement Improve an existing feature physics Physics performance monitoring labels Jul 1, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 1, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
100.0% Duplication on New Code

See analysis details on SonarQube Cloud

@stephenswat
Copy link
Member Author

stephenswat commented Jul 1, 2025

Performance summary

Here is a summary of the performance effects of this PR:

Graphical

Tabular

Kernel 673d408 e2c041d Delta
fit_forward 80.40 ms 82.52 ms 2.6%
propagate_to_next_surface 56.52 ms 56.50 ms -0.0%
fit_backward 40.55 ms 43.15 ms 6.4%
count_triplets 8.34 ms 8.34 ms -0.0%
find_tracks 4.28 ms 4.69 ms 9.5%
find_triplets 4.43 ms 4.44 ms 0.2%
count_doublets 1.70 ms 1.73 ms 1.5%
find_doublets 1.28 ms 1.27 ms -1.1%
fit_prelude 1.10 ms 1.10 ms 0.0%
ccl_kernel 870.63 μs 871.46 μs 0.1%
Thrust::sort 424.23 μs 424.08 μs -0.0%
select_seeds 349.30 μs 344.97 μs -1.2%
build_tracks 193.36 μs 193.29 μs -0.0%
update_triplet_weights 99.15 μs 99.91 μs 0.8%
apply_interaction 63.58 μs 63.49 μs -0.1%
fill_sort_keys 44.27 μs 44.31 μs 0.1%
estimate_track_params 34.47 μs 34.47 μs -0.0%
populate_grid 30.45 μs 30.41 μs -0.1%
count_grid_capacities 29.13 μs 29.12 μs -0.0%
unknown 20.51 μs 20.59 μs 0.4%
form_spacepoints 12.53 μs 12.45 μs -0.6%
reduce_triplet_counts 5.39 μs 5.39 μs 0.0%
make_barcode_sequence 1.02 μs 1.01 μs -0.3%
fill_prefix_sum 165.36 ns 165.36 ns -0.0%
Total 200.80 ms 205.94 ms 2.6%

Important

All metrics in this report are given as reciprocal throughput, not as wallclock runtime.

Note

This is an automated message produced on the explicit request of a human being.

@beomki-yeo
Copy link
Contributor

beomki-yeo commented Jul 1, 2025

The longer computation time is because there are more tracks which survive during their propagation? or is it purely from the increased amount of computation?

@stephenswat
Copy link
Member Author

stephenswat commented Jul 9, 2025

The longer computation time is because there are more tracks which survive during their propagation? or is it purely from the increased amount of computation?

I'm not entirely sure; the fact that propagate_to_next_surface doesn't change suggests the number of tracks is the same. Only the kernels that actually do the Kálmán update step increase so it seems to just be a computational change.

@krasznaa
Copy link
Member

krasznaa commented Jul 17, 2025

So, what's the idea with this PR? If it makes our calculations more stable, I would still try to get it in, even with the small performance penalty.

@stephenswat
Copy link
Member Author

stephenswat commented Jul 17, 2025

Yes I am happy go ahead with this. @niermann999 could you test if this fixes any of the KF problems you found? We should also merge in your KF tests ASAP.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 6, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@stephenswat

This comment was marked as outdated.

Sign in to view
@stephenswat
Switch Kálmán filters to use Joseph form
50776d5 
This commit switches the Kálmán filters to use the Joseph form
covariance update which is known to be usable for any gain matrix $K$
even if the gain matrix is distorted by numerical imprecision.
Empirically, this seems to resolve quite a few issues with our filtering
algorithms.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 7, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@stephenswat
Copy link
Member Author

stephenswat commented Nov 7, 2025

Physics performance summary

Here is a summary of the physics performance effects of this PR. Command used:

traccc_seeding_example_cuda --input-directory=/data/Acts/odd-simulations-20240506/geant4_ttbar_mu200 --digitization-file=geometries/odd/odd-digi-geometric-config.json --detector-file=geometries/odd/odd-detray_geometry_detray.json --grid-file=geometries/odd/odd-detray_surface_grids_detray.json --material-file=geometries/odd/odd-detray_material_detray.json --input-events=10 --use-acts-geom-source=on --check-performance --truth-finding-min-track-candidates=5 --truth-finding-min-pt=1.0 --truth-finding-min-z=-150 --truth-finding-max-z=150 --truth-finding-max-r=10 --seed-matching-ratio=0.99 --track-matching-ratio=0.5 --track-candidates-range=5:100 --seedfinder-vertex-range=-150:150

Seeding performance

Total number of seeds went from 298341 to 298345 (+0.0%)

Seeding plots



Track finding performance

Total number of found tracks went from 55970 to 55972 (+0.0%)

Finding plots









Track fitting performance

Fitting plots



















Note

This is an automated message produced on the explicit request of a human being.

niermann999
niermann999 approved these changes Nov 7, 2025
View reviewed changes
core/include/traccc/fitting/kalman_filter/two_filters_smoother.hpp
const matrix_type<D, D> i_minus_hk = I_m - H * K;
// See
// https://indico.cern.ch/event/1564924/contributions/6629447/attachments/3113201/5519076/asami_250731_acts.pdf
const matrix_type<D, D> R =
Copy link
Contributor

@niermann999 niermann999 Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should something like this also be done for R_smt? Or does that not make sense, since the smoothed covariance is calculated differently?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@niermann999 niermann999 niermann999 approved these changes

Assignees

No one assigned

Labels

improvement Improve an existing feature physics Physics performance monitoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stephenswat @beomki-yeo @krasznaa @niermann999